Skip to content

dylint: extended gotcha sweep (#826 — out-of-scope items)#837

Merged
zackees merged 1 commit into
mainfrom
feat/dylint-extended-826
Jun 29, 2026
Merged

dylint: extended gotcha sweep (#826 — out-of-scope items)#837
zackees merged 1 commit into
mainfrom
feat/dylint-extended-826

Conversation

@zackees

@zackees zackees commented Jun 29, 2026

Copy link
Copy Markdown
Member

Summary

Adds five new dylints, completing the #826 gotcha-sweep audit. Builds on PR #833 (which landed the first five lints + the websocket hardening). All five new lints follow the same pattern: own crate under dylints/, own rust-toolchain.toml (nightly-2026-03-26), trailofbits/dylint git rev 4bd91ce…, file-level allowlist with inline justifications.

New lints

Lint Diagnostic Allowlist
ban_process_exit_outside_main std::process::exit outside crates/*/src/main.rs and crates/*/src/bin/*.rs skips destructors — temp files leak, containment guards don't run, in-flight HTTP/WS responses get truncated. 9 legacy CLI subcommand dispatchers (build, deploy, dispatch, lnk, pio, purge, reset, serial_probe, show)
ban_unwrap_in_daemon_handlers .unwrap() inside an fbuild-daemon handler crashes the daemon and disconnects every connected client. Test files (*tests*.rs) and #[cfg(test)] modules are exempt (lint walks the owner chain). 3 legacy production files (websockets.rs, operations/build.rs, operations/deploy.rs) for serde_json::to_string / Response::builder().body() / static-table lookups that can't fail in practice — follow-up .unwrap_or_default() / .expect("…") migration
cli_no_build_deploy_direct_use fbuild-cli is a thin HTTP client — importing fbuild_build::* / fbuild_deploy::* bypasses the daemon. 6 diagnostic subcommands per crates/CLAUDE.md §"Diagnostic subcommand exception" (args, bloat_lookup, graph_cmd, symbols_cmd, compile_many, reset)
require_multi_thread_flavor_when_spawning #[tokio::test] on an async fn that calls tokio::spawn in its body must specify flavor = "multi_thread", otherwise spawned tasks deadlock under the default current_thread flavor. 2 legacy files (handlers/websockets_tests.rs, packages/install_lock.rs) — both rely on single-threaded ordering and need an explicit-synchronization rework
ban_std_sync_mutex_in_async std::sync::Mutex / std::sync::RwLock in crates/fbuild-daemon/src/** or crates/fbuild-serial/src/** — holding the guard across .await starves the Tokio worker; a panic poisons the lock. Test files and #[cfg(test)] modules exempt. 5 existing synchronous-discipline files (context.rs, device_manager.rs, status_manager.rs, handlers/operations/common.rs, manager.rs) — each with inline justification

Follow-up issues filed separately (not lints)

  • C1: audit: replace mock-based tests with integration tests … — best handled via CodeRabbit .coderabbit.yaml rules + human review, not dylint
  • C2: process: ignored-test policy + bitrot audit (#826 followup) — recommend a weekly grep-count posted to a tracking issue
  • C3: dylint: initializer-order lints (ban_env_var_set_after_import, require_oncelock_install_before_use) — design and implementation — both need global flow analysis (initializer ordering) that's out of scope for a single PR

Test plan

🤖 Generated with Claude Code

Adds five new dylints, completing the #826 audit:

- ban_process_exit_outside_main — `std::process::exit` only in
  `crates/*/src/main.rs` and `crates/*/src/bin/*.rs`. Skipping
  destructors leaks temp files, kills containment guards, and
  truncates in-flight HTTP/WS responses. Legacy CLI subcommand
  dispatchers (build/deploy/dispatch/lnk/pio/purge/reset/serial_probe/
  show) exempted via src/allowlist.txt.
- ban_unwrap_in_daemon_handlers — `.unwrap()` banned in
  `crates/fbuild-daemon/src/handlers/**`. Test files (*tests*.rs) and
  `#[cfg(test)]` modules are exempt; lint walks owner chain to detect
  the cfg(test) ancestor. Three legacy production sites
  (websockets.rs, operations/build.rs, operations/deploy.rs) are
  file-level allowlisted with follow-up notes.
- cli_no_build_deploy_direct_use — `fbuild-cli` is a thin HTTP client
  to the daemon; importing `fbuild_build::*` / `fbuild_deploy::*` is
  reserved for diagnostic subcommands (args/bloat_lookup/graph_cmd/
  symbols_cmd/compile_many/reset).
- require_multi_thread_flavor_when_spawning — `#[tokio::test]` on an
  async fn that calls `tokio::spawn` in its body must specify
  `flavor = "multi_thread"`, otherwise spawned tasks deadlock under
  the default `current_thread` flavor. Two legacy files
  (handlers/websockets_tests.rs, packages/install_lock.rs)
  allowlisted for follow-up migration.
- ban_std_sync_mutex_in_async — `std::sync::Mutex` /
  `std::sync::RwLock` banned in `crates/fbuild-daemon/src/**` and
  `crates/fbuild-serial/src/**`. Holding the guard across `.await`
  starves the Tokio worker; a panic poisons the lock for every
  subsequent caller. Test files and `#[cfg(test)]` modules exempt;
  existing synchronous-discipline uses (context.rs, device_manager.rs,
  status_manager.rs, handlers/operations/common.rs, manager.rs)
  allowlisted with inline justification.

All five lints follow the existing pattern from PR #833:

- Own crate under dylints/, own rust-toolchain.toml pin
  (nightly-2026-03-26), trailofbits/dylint at git rev 4bd91ce…
- Workspace excludes the crate so the stable workspace build is
  unaffected
- File-path-scoped detection via cx.sess().source_map() with
  slash-normalized matching
- File-level allowlist.txt with inline justifications
- Tests covering scope detection, allowlist suffix matching, and
  entry-point exemptions

Three additional items from #826's Tier 4 are not lints — they're
audit / process work tracked as separate follow-up issues:

- Mocks-as-primary-test-surface audit (recommend CodeRabbit rules,
  not dylint)
- Ignored-test bitrot policy (recommend weekly grep-count to
  tracking issue)
- Initializer-order lints (ban_env_var_set_after_import,
  require_oncelock_install_before_use) — need global flow analysis
  out of scope for this PR

Test plan:
- soldr cargo check --workspace --all-targets ✓
- soldr cargo clippy --workspace --all-targets -- -D warnings ✓
- soldr cargo test --workspace --no-fail-fast ✓ (all pass)
- `cargo dylint --all` validation deferred to CI on Ubuntu — the
  Windows host's Chocolatey rust shadow blocks local invocation
  (filed as zackees/soldr#1059)

See #826 for the original gotcha-sweep tracking issue
and PR #833 for the first five lints this builds on.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Jun 29, 2026

Copy link
Copy Markdown

Warning

Review limit reached

@zackees, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 44 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 1b5e6451-dc09-45ec-95d8-8af1821c61a8

📥 Commits

Reviewing files that changed from the base of the PR and between ca72d3f and f6785af.

📒 Files selected for processing (28)
  • Cargo.toml
  • ci/hooks/crate_guard.py
  • dylints/README.md
  • dylints/ban_process_exit_outside_main/Cargo.toml
  • dylints/ban_process_exit_outside_main/README.md
  • dylints/ban_process_exit_outside_main/rust-toolchain.toml
  • dylints/ban_process_exit_outside_main/src/allowlist.txt
  • dylints/ban_process_exit_outside_main/src/lib.rs
  • dylints/ban_std_sync_mutex_in_async/Cargo.toml
  • dylints/ban_std_sync_mutex_in_async/README.md
  • dylints/ban_std_sync_mutex_in_async/rust-toolchain.toml
  • dylints/ban_std_sync_mutex_in_async/src/allowlist.txt
  • dylints/ban_std_sync_mutex_in_async/src/lib.rs
  • dylints/ban_unwrap_in_daemon_handlers/Cargo.toml
  • dylints/ban_unwrap_in_daemon_handlers/README.md
  • dylints/ban_unwrap_in_daemon_handlers/rust-toolchain.toml
  • dylints/ban_unwrap_in_daemon_handlers/src/allowlist.txt
  • dylints/ban_unwrap_in_daemon_handlers/src/lib.rs
  • dylints/cli_no_build_deploy_direct_use/Cargo.toml
  • dylints/cli_no_build_deploy_direct_use/README.md
  • dylints/cli_no_build_deploy_direct_use/rust-toolchain.toml
  • dylints/cli_no_build_deploy_direct_use/src/allowlist.txt
  • dylints/cli_no_build_deploy_direct_use/src/lib.rs
  • dylints/require_multi_thread_flavor_when_spawning/Cargo.toml
  • dylints/require_multi_thread_flavor_when_spawning/README.md
  • dylints/require_multi_thread_flavor_when_spawning/rust-toolchain.toml
  • dylints/require_multi_thread_flavor_when_spawning/src/allowlist.txt
  • dylints/require_multi_thread_flavor_when_spawning/src/lib.rs
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feat/dylint-extended-826

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@zackees zackees merged commit e1b4486 into main Jun 29, 2026
84 of 93 checks passed
@zackees zackees deleted the feat/dylint-extended-826 branch June 29, 2026 19:08
zackees added a commit that referenced this pull request Jun 29, 2026
Releases the post-#813 / #802 / #826 / #844 work landed across:
- PR #843 — workspace-wide timeout sweep (#802 family)
- PR #837, #849, #850, #851 — internal-bridges + dylint sweep (#826 + #844)
- PR #842 — async migration follow-ups (#813)

15 dylints now ship in dylints/ (4 from earlier, 10 from #826/#844,
plus the renamed ban_unwrap_in_production). 7 internal bridge APIs:
fbuild_core::{http, fs, time, channel, path::canonicalize_existing},
fbuild_paths::temp_subdir, fbuild_cli::output.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@fastled-project-sync fastled-project-sync Bot moved this to Triage in FastLED Tracker Jun 30, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Triage

Development

Successfully merging this pull request may close these issues.

1 participant